Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Gamm stableswap improvements #3839

Merged
merged 6 commits into from
Jan 3, 2023
Merged

Gamm stableswap improvements #3839

merged 6 commits into from
Jan 3, 2023

Conversation

ljah8
Copy link
Contributor

@ljah8 ljah8 commented Dec 22, 2022

What is the purpose of the change

This PR is created during the Informal Systems audit, after analysis of the existing specification and code inspection
Auditing is performed on commit hash: 7374795
This PR contains minor Stableswap code improvements within the GAMM module noticed during code inspection with the aim of finding issues.

Brief Changelog

  • stableswap/pool.go:
    • Part of the code used to build errors is moved to errors.go;
    • The getLiquidityIndexMap and GetScalingFactorByLiquidityIndex functions are replaced by the GetScalingFactorByDenom function to get the appropriate scaling factor faster and avoid creating a map. While implementing the new function, we didn't want to change logic, so it doesn't return an error or panic if no scaling factor is found.
    • applyScalingFactorMultiplier function is protected against unwanted overflow in case multiplier > 1
  • Minor changes in the implementation of BinarySearchBigDec, BinarySearch (osmoutils/binary_search.go) and joinPoolSharesInternal (stableswap/amm.go) functions.
  • Removed redundant error from iterKCalculator function that always returned nil.
  • Added checking for potential division by zero in Compare and CompareBigDec functions.

Testing and Verifying

  • Added a unit test covering the GetScalingFactorByDenom function.
  • Existing tests are modified in accordance with the new changes.

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? no
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? no
  • How is the feature or change documented? not applicable

@ljah8 ljah8 added the V:state/compatible/backport State machine compatible PR, should be backported label Dec 22, 2022
@ljah8 ljah8 requested a review from ValarDragon December 22, 2022 14:32
@github-actions github-actions bot added C:docs Improvements or additions to documentation C:x/gamm Changes, features and bugs related to the gamm module. labels Dec 22, 2022
AlpinYukseloglu
AlpinYukseloglu approved these changes Dec 22, 2022
View reviewed changes
Copy link
Contributor

@AlpinYukseloglu AlpinYukseloglu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the cleanup!

@ValarDragon ValarDragon added V:state/compatible/no_backport State machine compatible PR, depends on prior breaks and removed V:state/compatible/backport State machine compatible PR, should be backported labels Jan 3, 2023
@ValarDragon
Copy link
Member

LGTM! I had previously suggested backporting this.

Even though this looks like it should be fine, changing to no backport just to be abundandantly cautious with the division by zero change. (I'm pretty sure it should be impossible to get a zero in current code)

@ValarDragon ValarDragon merged commit 2ac5d35 into osmosis-labs:main Jan 3, 2023
@czarcas7ic czarcas7ic mentioned this pull request Jan 5, 2023
Merged
@ValarDragon ValarDragon added the A:backport/v14.x backport patches to v14.x branch label Jan 6, 2023
@mergify mergify bot mentioned this pull request Jan 6, 2023
Merged
mergify bot pushed a commit that referenced this pull request Jan 6, 2023
@ljah8 @mergify
Gamm stableswap improvements (#3839)
1c14a18 
* Stableswap: README pseudocode fixes

* Stableswap: Binary search code improvement

* Stableswap: minor code improvements

* Stableswap: minor code improvements 2

* Binary search: Check potential division by zero

(cherry picked from commit 2ac5d35)

# Conflicts:
#	x/gamm/pool-models/stableswap/util_test.go
p0mvn pushed a commit that referenced this pull request Jan 7, 2023
@mergify @ljah8 @ValarDragon
Gamm stableswap improvements (backport #3839) (#3933)
1067f1e 
* Gamm stableswap improvements (#3839)

* Stableswap: README pseudocode fixes

* Stableswap: Binary search code improvement

* Stableswap: minor code improvements

* Stableswap: minor code improvements 2

* Binary search: Check potential division by zero

(cherry picked from commit 2ac5d35)

# Conflicts:
#	x/gamm/pool-models/stableswap/util_test.go

* Fix conflict

* Update osmomath go mod

Co-authored-by: Aleksandar Ljahović <[email protected]>
Co-authored-by: Dev Ojha <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlpinYukseloglu AlpinYukseloglu AlpinYukseloglu approved these changes

@ValarDragon ValarDragon Awaiting requested review from ValarDragon

Assignees
No one assigned
Labels
A:backport/v14.x backport patches to v14.x branch C:docs Improvements or additions to documentation C:x/gamm Changes, features and bugs related to the gamm module. V:state/compatible/no_backport State machine compatible PR, depends on prior breaks
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ljah8 @ValarDragon @AlpinYukseloglu